fix(node): Correctly resolve module name #10001

timfish · 2023-12-31T19:21:22Z

Caches the basePath so dirname(require.main.filename) only gets called once
Now uses full path for module name
Now correctly handles inside node_modules
Add more tests

mydea · 2024-01-02T08:22:31Z

packages/node/src/module.ts

+
+function getBasePath(): string {
+  if (!basePath) {
+    const baseDir =


l: This is not really related to this PR, but it would be nice to have a comment explaining what & why we actually do here - e.g. why use require.main.filename over global.process.cwd() etc. Feel free to ignore this, but as somebody with little to no historical context on this, this appears very magic to me 😅

I have no idea other than this has been like this for 11+ years from raven-node:
https://github.com/getsentry/raven-node/blame/7019cb30efda40704105b7b510bc428ec0c1ee01/lib/utils.js#L87

It is a good point though which is worth considering as it will be important for #9072.

I guess the issue with process.cwd() is that it might give us the repository root rather than the runtime root. For example, the app might be run from process.cwd() + '/packages/my-app/dist/' so if we use cwd, paths (and therefore modules, will always start packages.my-app.dist.*.

dirname(require.main.filename) will only give us an accurate runtime root if the entry point is in the root. I suspect this is true the majority of the time.

In the Deno SDK, due to permissions we might not have have access to cwd() or the file system. To work around this, we parse a new Error().stack from init and pick the common root path from the stack trace.

sentry-javascript/packages/deno/src/integrations/normalizepaths.ts

Lines 7 to 40 in ff95fd5

function appRootFromErrorStack(error: Error): string | undefined {

// We know at the other end of the stack from here is the entry point that called 'init'

// We assume that this stacktrace will traverse the root of the app

const frames = createStackParser(nodeStackLineParser())(error.stack || '');

const paths = frames

// We're only interested in frames that are in_app with filenames

.filter(f => f.in_app && f.filename)

.map(

f =>

(f.filename as string)

.replace(/^[A-Z]:/, '') // remove Windows-style prefix

.replace(/\\/g, '/') // replace all `\` instances with `/`

.split('/')

.filter(seg => seg !== ''), // remove empty segments

) as string[][];

if (paths.length == 0) {

return undefined;

}

if (paths.length == 1) {

// Assume the single file is in the root

return dirname(paths[0].join('/'));

}

// Iterate over the paths and bail out when they no longer have a common root

let i = 0;

while (paths[0][i] && paths.every(w => w[i] === paths[0][i])) {

i++;

}

return paths[0].slice(0, i).join('/');

}

mydea · 2024-01-02T08:23:00Z

💯 01, nice!

fix(node): Correctly resolve module name

8179837

timfish force-pushed the fix/node-module-name branch from 06636e5 to 8179837 Compare December 31, 2023 19:38

timfish marked this pull request as ready for review January 1, 2024 13:57

mydea reviewed Jan 2, 2024

View reviewed changes

mydea approved these changes Jan 2, 2024

View reviewed changes

AbhiPrasad merged commit e8e6ab8 into getsentry:develop Jan 2, 2024
55 checks passed

timfish deleted the fix/node-module-name branch January 2, 2024 15:54

anonrig pushed a commit that referenced this pull request Jan 3, 2024

fix(node): Correctly resolve module name (#10001)

7ef76de

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(node): Correctly resolve module name #10001

fix(node): Correctly resolve module name #10001

timfish commented Dec 31, 2023 •

edited

Loading

mydea Jan 2, 2024

timfish Jan 2, 2024 •

edited

Loading

timfish Jan 2, 2024

mydea commented Jan 2, 2024 •

edited

Loading

	function appRootFromErrorStack(error: Error): string \| undefined {
	// We know at the other end of the stack from here is the entry point that called 'init'
	// We assume that this stacktrace will traverse the root of the app
	const frames = createStackParser(nodeStackLineParser())(error.stack \|\| '');

	const paths = frames
	// We're only interested in frames that are in_app with filenames
	.filter(f => f.in_app && f.filename)
	.map(
	f =>
	(f.filename as string)
	.replace(/^[A-Z]:/, '') // remove Windows-style prefix
	.replace(/\\/g, '/') // replace all `\` instances with `/`
	.split('/')
	.filter(seg => seg !== ''), // remove empty segments
	) as string[][];

	if (paths.length == 0) {
	return undefined;
	}

	if (paths.length == 1) {
	// Assume the single file is in the root
	return dirname(paths[0].join('/'));
	}

	// Iterate over the paths and bail out when they no longer have a common root
	let i = 0;
	while (paths[0][i] && paths.every(w => w[i] === paths[0][i])) {
	i++;
	}

	return paths[0].slice(0, i).join('/');
	}

fix(node): Correctly resolve module name #10001

fix(node): Correctly resolve module name #10001

Conversation

timfish commented Dec 31, 2023 • edited Loading

mydea Jan 2, 2024

Choose a reason for hiding this comment

timfish Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

timfish Jan 2, 2024

Choose a reason for hiding this comment

mydea commented Jan 2, 2024 • edited Loading

timfish commented Dec 31, 2023 •

edited

Loading

timfish Jan 2, 2024 •

edited

Loading

mydea commented Jan 2, 2024 •

edited

Loading